Replace Spline With Ramp For Shader Parameters #1499
Conversation
danieldresser-ie
force-pushed
the
splinesRound2
branch
from
4fb3a18 to
c198a76
Compare
|
Huh, the build failure on Mac OS is pretty weird ... it's in a file I shouldn't have affected at all, and don't immediately see any way I've touched ... it's in CurvesPrimitiveEvaluator ( the other sort of spline ), and it appears to be a naming clash with something named "linear", but the only thing I'm aware of having added named linear is IECore::RampInterpolation::Linear ( with a capital, in a namespace, and not even included here? ). Odd. |
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
Thanks Daniel! A few comments inline - I think the main topic of discussion is how doable it would be to move some of the Ramp stuff to ShaderNetworkAlgo...perhaps we can talk about that in the meeting that started one minute ago :)
When converting a ramp to OSL conventions, is the variable holding it still named "ramp", or is it called a "spline" once it's converted?
I'd be inclined to call the OSL variables spline*, to indicate the transformation.
There's an outstanding TODO in src/IECoreGL/ShaderStateComponent.cpp, which currently converts SplineData to textures...but if it's only being used elsewhere, then maybe anyone who is still using it would benefit more from keeping the old behaviour?
That would be my guess - I think the most likely client would also be using SplineParameter. Maybe folks at IE will know more, but I don't know of any Gaffer-side uses.
Huh, the build failure on Mac OS is pretty weird .
Yeah, this is nothing to do with the PR - it was broken previously. My bad - I merged a PR from Paul without noticing the failure on Mac, and need to find time to fix it (since we aren't shipping anything on Mac, it's not urgent).
|
One more quick note - this PR needs to target |
The actual classes were removed long ago in 10.0.0-a12
danieldresser-ie
force-pushed
the
splinesRound2
branch
from
c198a76 to
adb8516
Compare
|
Addressed all feedback ( aside from the oslStartPointMultiplicity / oslStartPointDuplication naming/off-by-one issue that I realized at the last minute probably needs one last tweak ). Retargeted to main. Added a couple of Changelog entries ready to be squashed in ( oh, and I should probably add a separate changelog entry to flag the deprecations in de8f2b9 ).
I attempted to follow this convention with 15b62d4. I'm not fully convinced I like this, it feels a bit confusing having to reason about exactly what point in the conversion process it starts being named spline, but think it's fairly consistent now. |
|
Thanks Daniel - updates all look good to me. I think we do still want to update the |
danieldresser-ie
force-pushed
the
splinesRound2
branch
from
08c961d to
5ec874f
Compare
|
Updated oslStartPointMultiplicity() semantics, added Changelog entry for ShaderNetworkAlgo deprecated function removals, and squashed. |
2 participants
We shouldn't merge this before we fully sort out the Gaffer side as well, to make sure that everything works with the new API ( I think I've got everything working in Gaffer now, but it's not cleaned up enough to put the PR up yet ). But the Cortex side should be good to review now.
Known questions currently: